Add Grandstream Home integration#167872
Conversation
d30e08d to
fef1454
Compare
8aad180 to
39a400b
Compare
|
The source for the library is missing: https://github.com/GrandstreamEngineering/grandstream_home_api |
f443b7c to
aa77908
Compare
It is added now |
aa77908 to
8a3b791
Compare
8a3b791 to
0b3e604
Compare
edenhaus
left a comment
There was a problem hiding this comment.
I did an initial review until I stopped as this PR is to huge. Some code needs to move to the library and also the config flow is huge. Please refactor the code and make it more readable.
Also the library needs a working workflow to deploy directly to pypi.
As 10.000 changes is to much, can we just add a single device/model in this PR and then add other models in a follow up PR. This PR needs to be a lot smaller
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
94772a0 to
62a2b83
Compare
62a2b83 to
3adbbd7
Compare
3adbbd7 to
a5c791e
Compare
| # Reauthentication Flow | ||
| async def async_step_reauth( |
There was a problem hiding this comment.
Please keep reauth out of the first PR
| # Store auth info | ||
| self._auth_info = { | ||
| CONF_USERNAME: username, | ||
| CONF_PASSWORD: encrypt_password(password, self.unique_id or "default"), |
There was a problem hiding this comment.
Why do we encrypt the password?
| # Initialize global API lock if not exists | ||
| hass.data.setdefault(DOMAIN, {}) | ||
| if "api_lock" not in hass.data[DOMAIN]: | ||
| hass.data[DOMAIN]["api_lock"] = asyncio.Lock() |
There was a problem hiding this comment.
Why do we need a global API lock?
| if error_type == "offline": | ||
| _LOGGER.warning("API login failed (device may be offline)") | ||
| return |
There was a problem hiding this comment.
Why does the device need to login to the API?
| if error_type == "ha_control_disabled": | ||
| raise ConfigEntryAuthFailed( | ||
| "Home Assistant control is disabled on the device. " | ||
| "Please enable it in the device web interface." | ||
| ) |
There was a problem hiding this comment.
Why can HA control be disabled?
| async def async_handle_push_data(self, data: dict[str, Any]) -> None: | ||
| """Handle pushed data.""" | ||
| try: | ||
| _LOGGER.debug("Received push data: %s", data) | ||
| data = process_push_data(data) | ||
| self.last_update_method = "push" | ||
| self.async_set_updated_data(data) | ||
| except Exception as e: | ||
| _LOGGER.error("Error processing push data: %s", e) | ||
| raise | ||
|
|
||
| def handle_push_data(self, data: dict[str, Any]) -> None: | ||
| """Handle push data synchronously.""" | ||
| try: | ||
| _LOGGER.debug("Processing sync push data: %s", data) | ||
| data = process_push_data(data) | ||
| self.last_update_method = "push" | ||
| self.async_set_updated_data(data) | ||
| except Exception as e: | ||
| _LOGGER.error("Error processing sync push data: %s", e) | ||
| raise |
There was a problem hiding this comment.
I am not sure what the task of this class is, should this be a base entity instead?
| "type": "_https._tcp.local." | ||
| }, | ||
| { | ||
| "name": "*", |
There was a problem hiding this comment.
I think matching on asterisk is too broad
| # Add SIP account sensors (only if accounts exist) | ||
| # Track by account ID instead of index | ||
| sip_accounts = coordinator.data.get("sip_accounts", []) | ||
| for account in sip_accounts: | ||
| if isinstance(account, dict): | ||
| account_id = account.get("id", "") | ||
| if account_id: | ||
| entities.extend( | ||
| GrandstreamSipAccountSensor( | ||
| coordinator, device, description, account_id | ||
| ) | ||
| for description in SIP_ACCOUNT_SENSORS | ||
| ) | ||
| created_sip_sensors.add(account_id) | ||
|
|
||
| # Add listener to dynamically add new SIP account sensors | ||
| @callback | ||
| def _async_add_sip_sensors() -> None: | ||
| """Add new SIP account sensors when accounts are added.""" | ||
| sip_accounts = coordinator.data.get("sip_accounts", []) | ||
| new_entities: list[GrandstreamSipAccountSensor] = [] | ||
|
|
||
| for account in sip_accounts: | ||
| if isinstance(account, dict): | ||
| account_id = account.get("id", "") | ||
| if account_id and account_id not in created_sip_sensors: | ||
| new_entities.extend( | ||
| GrandstreamSipAccountSensor( | ||
| coordinator, device, description, account_id | ||
| ) | ||
| for description in SIP_ACCOUNT_SENSORS | ||
| ) | ||
| created_sip_sensors.add(account_id) | ||
|
|
||
| if new_entities: | ||
| async_add_entities(new_entities) | ||
|
|
||
| # Register listener | ||
| config_entry.async_on_unload( | ||
| coordinator.async_add_listener(_async_add_sip_sensors) | ||
| ) | ||
|
|
There was a problem hiding this comment.
Can we keep dynamic devices out of this PR
There was a problem hiding this comment.
Can we cut this down to a single type of information first and add the rest in later PRs? I think that will make this much more reviewable
This integration adds support for Grandstream GDS/GNS/GSC devices.
Features:
Breaking change
Proposed change
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: